Skip to content

fix(lsp): context is singleton in vscode#4722

Merged
benfdking merged 1 commit into
mainfrom
start_with_broken_test
Jun 11, 2025
Merged

fix(lsp): context is singleton in vscode#4722
benfdking merged 1 commit into
mainfrom
start_with_broken_test

Conversation

@benfdking
Copy link
Copy Markdown
Contributor

No description provided.

@benfdking benfdking requested a review from Copilot June 11, 2025 18:00

This comment was marked as outdated.

@benfdking benfdking force-pushed the start_with_broken_test branch from 04201bc to 27a2fae Compare June 11, 2025 18:01
@benfdking benfdking requested a review from Copilot June 11, 2025 18:01

This comment was marked as outdated.

@benfdking benfdking force-pushed the start_with_broken_test branch from 27a2fae to b720d5f Compare June 11, 2025 18:05
@benfdking benfdking requested a review from Copilot June 11, 2025 18:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the behavior of the LSP context by ensuring it is treated as a singleton in VSCode. The changes update context creation, context reloading in document events, and ensure consistency when a new context is initialized.

Comment thread sqlmesh/lsp/main.py
Comment on lines +109 to +115
if self.lsp_context is None:
context = self.context_class(paths=paths)
else:
self.lsp_context.context.load()
context = self.lsp_context.context
self.lsp_context = LSPContext(context)
return self.lsp_context
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current logic re-wraps the context in a new LSPContext even when one exists. To fully honor the singleton pattern, consider reusing the existing LSPContext instance when no state updates are required.

Copilot uses AI. Check for mistakes.
Comment thread sqlmesh/lsp/main.py
@self.server.feature(types.TEXT_DOCUMENT_DID_CHANGE)
def did_change(ls: LanguageServer, params: types.DidChangeTextDocumentParams) -> None:
if self.lsp_context is None:
current_path = Path.cwd()
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Path.cwd() might be fragile if the server's working directory does not reflect the intended project folder. Consider deriving the path from the document URI or a configuration parameter to ensure the correct context is used.

Suggested change
current_path = Path.cwd()
current_path = self._get_workspace_folder_or_default()

Copilot uses AI. Check for mistakes.
Comment thread sqlmesh/lsp/main.py
context = self.lsp_context
context.context.load() # Reload or refresh context
self.lsp_context = LSPContext(context.context)
self.lsp_context.context.load()
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The context reinitialization logic is similar to what is done in _create_lsp_context. Consider refactoring this duplication into a helper method to ensure consistent behavior and easier maintenance.

Copilot uses AI. Check for mistakes.
@benfdking benfdking merged commit a9ef531 into main Jun 11, 2025
25 checks passed
@benfdking benfdking deleted the start_with_broken_test branch June 11, 2025 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants